Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new EventSchema and SchemaRegistry API #4

Merged
merged 22 commits into from
Aug 11, 2022

Conversation

Zsailer
Copy link
Member

@Zsailer Zsailer commented Aug 9, 2022

This proceeds the work in #2. The review in that PR became focused on "categories" and "redactionPolicies". I'm open this PR here to keep that discussion in tact while moving the review of the basic API design (minus the categories stuff) here.

Highlights

  • Adds EventSchema API for validating schemas and events.
  • Adds SchemaRegistry to cache validators for all registered schemas. This prevents a memory leak I have been seeing this library.
  • Validates all schemas against a meta schema to enforce Jupyter Events schema structure.
  • Removed "categories" handling (for now), since this is more of a telemetry use-case.
  • Defaults to emitting all events (opposite of Jupyter Telemetry).

Public API changes

  • "categories" is no longer used for redacting sensitive information. I
  • "allowed_schemas", "allowed_categories", and "allowed_properties", have been removed from the EventLogger.
    • All registered event types will be emitted.
    • The expectation is that consumers will use filters in the logging handlers to listen for specific events.
  • EventLogger.record_event has been renamed to EventLogger.emit.

@Zsailer Zsailer added the enhancement New feature or request label Aug 9, 2022
@Zsailer
Copy link
Member Author

Zsailer commented Aug 9, 2022

This PR is ready for final review.

The only test that's failing is the "check-links" test in the documentation, which is failing because the new docs refer to links that will be created once this PR is merged.

Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APIs look good to me, just one minor question

jupyter_events/yaml.py Outdated Show resolved Hide resolved
@Zsailer
Copy link
Member Author

Zsailer commented Aug 10, 2022

Thanks, @blink1073! I addressed various comments from @kevin-bates in #2 too. I'll likely merge this here work on getting the documentation rendering. Then, I'll shift to integrating with Jupyter Server.

Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Zsailer - this looks good. Sorry about focusing on #2 rather than this PR. Looks like the majority of comments are not applicable in this version (nice!). This is the only comment that's tbd, but that's something that we can look into later and/or doesn't affect the service overall.

@Zsailer
Copy link
Member Author

Zsailer commented Aug 10, 2022

@kevin-bates I think I addressed that comment in this commit: 8d43c5b

@kevin-bates
Copy link
Member

Thanks @Zsailer. It looks like that warning will (also) be issued if there are no handlers, yet have registered (and matching) schema/version pairs - which, I suspect, will be common and probably not worthy of a warning. I was thinking only the missing schema/version pair scenario would be logged.

@Zsailer
Copy link
Member Author

Zsailer commented Aug 10, 2022

Ah, yes. Thanks, Kevin! Fixed in 4659ddf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants